ESQL: Simplify TableIdentifier class + rename to IndexPattern #120797
Conversation
This was always null throughout the entire code base.
alex-spies
added
>refactoring
auto-backport
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| import org.elasticsearch.xpack.esql.plan.TableIdentifier; | ||
| import org.elasticsearch.xpack.esql.plan.IndexPattern; | ||
|
|
||
| public class TableInfo { |
There was a problem hiding this comment.
In a future PR, I think this class just has to go. It is a wrapper around TableIdentifier, except that it throws away the equality/hash methods and uses instance identity for equality.
|
|
||
| import static org.elasticsearch.transport.RemoteClusterAware.REMOTE_CLUSTER_INDEX_SEPARATOR; | ||
|
|
||
| public class TableIdentifier { |
There was a problem hiding this comment.
This doesn't show properly in the diff, but the changes here are simple:
- Rename to
IndexPattern - Remove the
cluster/catalogfield (entirely unused, was always null in all constructor calls) - Remove the
cluster()andqualifiedIndexmethods (both entirely unused) - Adapt
equalsandhashCode
.../plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/UnresolvedRelation.java
Outdated
Show resolved
Hide resolved
| public class IndexPattern { | ||
|
|
||
| private final Source source; | ||
| private final String indexPattern; |
There was a problem hiding this comment.
I wonder if this should be called indexPattern(s) as it might list coma separated patterns?
Not sure if it is needed often, but #120277 would benefit if it is possible to return list/array of separate patterns.
There was a problem hiding this comment.
We can add a helper method as part of #120277! I'd like to not add logic in this PR, so it remains a straight refactoring.
There was a problem hiding this comment.
Also, yeah, indexPatterns (plural) would be even more precise - but then we'd have to align e.g. EsRelation as well. And it's also reasonable to consider idx,logs-* a single pattern which matches idx and anything starting with logs-.
Rename IndexPattern.index to indexPattern and UnresolvedRelation.table to indexPattern.
alex-spies
added
the
auto-merge-without-approval
💔 Backport failed
You can use sqren/backport to manually backport by running |
…c#120797) This class is confusing: - It contains an **unused** `cluster` attribute - we never separate out the cluster, it remains in the `index` field. Also, in the constructor, this field is called `catalog`, which is a concept entirely absent from ESQL at the moment. - It can refer to multiple indices, even multiple wildcard patterns, but doesn't mention this neither in its name nor javadoc. - It has little to do with tables, which is likely a remnant of this class' usage in SQL, before the `esql.core` split. This PR removes the `cluster` attribute, renames the class to `IndexPattern`, and adds javadoc to clarify that it can also contain stuff like `remote1:idx1,remote-*:idx-*`. (cherry picked from commit 060c833)
… (#120884) This class is confusing: - It contains an **unused** `cluster` attribute - we never separate out the cluster, it remains in the `index` field. Also, in the constructor, this field is called `catalog`, which is a concept entirely absent from ESQL at the moment. - It can refer to multiple indices, even multiple wildcard patterns, but doesn't mention this neither in its name nor javadoc. - It has little to do with tables, which is likely a remnant of this class' usage in SQL, before the `esql.core` split. This PR removes the `cluster` attribute, renames the class to `IndexPattern`, and adds javadoc to clarify that it can also contain stuff like `remote1:idx1,remote-*:idx-*`. (cherry picked from commit 060c833) Co-authored-by: Alexander Spies <[email protected]>
3 participants
This class is confusing:
clusterattribute - we never separate out the cluster, it remains in theindexfield. Also, in the constructor, this field is calledcatalog, which is a concept entirely absent from ESQL at the moment.esql.coresplit.This PR removes the
clusterattribute, renames the class toIndexPattern, and adds javadoc to clarify that it can also contain stuff likeremote1:idx1,remote-*:idx-*.